Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
There was a problem hiding this comment.
Pull request overview
Enables building the health monitoring Rust crate under cfg(loom) by excluding existing unit tests from loom builds, wiring in the loom dependency, and adding a Bazel target intended for loom-based testing.
Changes:
- Gate existing Rust unit-test modules behind
#[cfg(all(test, not(loom)))]to avoid compiling them under loom. - Add
loomas a dependency forcfg(loom)builds and whitelistcfg(loom)via workspaceunexpected_cfgslint configuration. - Add a Bazel
loom_testsrust_testtarget and pinscore_cratesviagit_overrideto obtain the needed Bazel crate(s).
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/health_monitoring_lib/rust/worker.rs | Excludes this module’s unit tests from loom builds. |
| src/health_monitoring_lib/rust/tag.rs | Excludes this module’s unit tests from loom builds. |
| src/health_monitoring_lib/rust/lib.rs | Excludes crate-level unit tests from loom builds. |
| src/health_monitoring_lib/rust/ffi.rs | Excludes FFI unit tests from loom builds. |
| src/health_monitoring_lib/rust/deadline/ffi.rs | Excludes deadline FFI unit tests from loom builds. |
| src/health_monitoring_lib/rust/deadline/deadline_state.rs | Excludes deadline state unit tests from loom builds. |
| src/health_monitoring_lib/rust/deadline/deadline_monitor.rs | Excludes deadline monitor unit tests from loom builds. |
| src/health_monitoring_lib/rust/deadline/common.rs | Excludes deadline common unit tests from loom builds. |
| src/health_monitoring_lib/Cargo.toml | Adds loom dependency under cfg(loom). |
| src/health_monitoring_lib/BUILD | Adds loom_tests Bazel target that builds with --cfg loom. |
| MODULE.bazel | Pins score_crates to a specific commit via git_override. |
| Cargo.toml | Whitelists cfg(loom) in workspace rust lints to avoid unexpected_cfgs warnings. |
| Cargo.lock | Locks additional transitive deps introduced by loom. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rust_test( | ||
| name = "loom_tests", | ||
| crate = ":health_monitoring_lib", | ||
| crate_features = ["stub_supervisor_api_client"], | ||
| rustc_flags = [ | ||
| "--cfg", | ||
| "loom", | ||
| "-C", | ||
| "link-arg=-lm", | ||
| "-C", | ||
| "link-arg=-lc", | ||
| ] + select({ | ||
| "@platforms//os:qnx": [ | ||
| "-Clink-arg=-lc++", | ||
| ], | ||
| "@platforms//os:linux": [ | ||
| "-Clink-arg=-lrt", | ||
| "-Clink-arg=-lstdc++", | ||
| ], | ||
| }), | ||
| deps = [ | ||
| "@score_baselibs_rust//src/log/stdout_logger", | ||
| "@score_crates//:loom", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
loom_tests compiles the crate with --cfg loom, but all in-crate unit test modules are now gated behind #[cfg(all(test, not(loom)))] (and there are no #[cfg(all(test, loom))] tests in the crate). As a result this Bazel test target will run zero Rust tests and can give a false sense of coverage. Consider adding at least one loom-specific test module (or renaming this target to indicate it's a compile/link check rather than a test suite).
MODULE.bazel
Outdated
| bazel_dep(name = "score_crates", version = "0.0.7") | ||
| git_override( | ||
| module_name = "score_crates", | ||
| commit = "c735d9fe12f255c2ae427eeb87b71f2cb46d015f", | ||
| remote = "https://github.com/eclipse-score/score-crates.git", | ||
| ) | ||
|
|
There was a problem hiding this comment.
The score_crates dependency is now pinned via git_override to a specific commit, but there’s no inline note explaining why the registry version (0.0.7) is insufficient. Since overrides affect reproducibility and upgradeability, add a short comment describing the required fix/feature (e.g., loom support) and, if possible, prefer bumping the bazel_dep version to a release that contains it instead of a raw commit pin.
|
The created documentation from the pull request is available at: docu-html |
289b9bb to
16f80df
Compare
16f80df to
5e8e564
Compare
5e8e564 to
4c9ca89
Compare
Enable `loom` in health monitor.
4c9ca89 to
e407f95
Compare
Enable
loomin health monitor.